-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use extract_argument
for #[setter]
extraction
#3998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple of suggestions for this one...
} | ||
}; | ||
|
||
let holder = holders.push_holder(span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a TODO to rework this to use impl_arg_param
later? I think the hardest part to doing so would be that impl_arg_param
assumes the arguments are in an array and takes the array ident plus an index. We could probably rework that so that impl_arg_param
just gets a TokenStream
of something to extract from.
(Of course, if you're up for it, can also go for the refactoring now 👀)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added, the TODO for now, I think if we refactor this, this should also get rid of the from_py_with
case, since that is handled in impl_arg_param
already. I think it makes sense to do that, for consistency and code deduplication, but probably deserves it's own PR. (We would also need to figure out how to handle the Descriptor
case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it would be a fair bit of rewiring, hopefully with a rewarding payoff at the end! 👍
690c5e0
to
1243232
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! 👍
Following #3995 (comment)
This updates the
#[setter]
argument toextract_argument
instead ofFromPyObject::extract_bound
. This allows extracting the same types as normal#[pyfunction]
arguments, in particular&Bound
. This also adds the deprecation warnings for gil-ref extraction.